fix: handle PEP 604 unions (X | Y) like Union[X, Y] - #746
Merged
Conversation
`get_origin(int | None)` is `types.UnionType`, not `typing.Union`, on
python < 3.14 -- so the four `get_origin(...) is Union` comparisons
silently took the non-union path for PEP 604 annotations.
Most visibly, the Optional wrapper was not stripped from a widget's
reported annotation:
@magicgui
def f(x: Optional[int] = None): ... # .annotation -> int
@magicgui
def f(x: int | None = None): ... # .annotation -> int | None
and `register_type(int | str, return_callback=...)` registered nothing
for the individual member types.
Adds `magicgui._util.is_union`, which accepts both spellings, and uses
it at all four sites.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #746 +/- ##
==========================================
+ Coverage 89.49% 89.52% +0.03%
==========================================
Files 40 40
Lines 4893 4899 +6
==========================================
+ Hits 4379 4386 +7
+ Misses 514 513 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The preceding fix makes `X | None` behave like `Optional[X]`, so the pyupgrade rewrite is safe for src. Scoped deliberately: - UP045 is enabled for src/ and docs/ only; tests/ keep it ignored, since they exercise both spellings on purpose (see test_no_order). - UP007 stays ignored: `Union` is still needed as a runtime *value* for the public type aliases (PathLike, ChoicesType, AppRef, TableData, WidgetRef) and for `Union[args]` construction. ruff offers no fix for those 11 sites, so enabling it would just leave permanent errors. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #745, which uncovered this.
The bug
get_originreports a different origin for the two union spellings on python < 3.14:magicgui compared
get_origin(...) is Unionin four places, so every PEP 604 annotation quietly took the non-union path. Two user-visible consequences:Widget selection was unaffected —
_split_annotation_typeand_literal_choicesuseget_args()directly, which works for both spellings. Only the fouris Unioncomparisons were wrong, which is why this stayed hidden. Masked entirely on 3.14, wheretypes.UnionTypebecametyping.Union.The fix
Adds
magicgui._util.is_union, which accepts both spellings, and uses it at all four sites (_type_map,_ui_field×2,_value_widget).Re-enabling the pyupgrade rewrite
#745 put
UP007/UP045in ruff's ignore list because of the bug above. With it fixed,UP045is safe and is now enabled — but scoped deliberately:UP045on forsrc/anddocs/, off fortests/. Tests exercise both spellings on purpose; blanket-rewriting them would cost coverage (this is what neuteredtest_no_orderin ci(pre-commit.ci): autoupdate #728).UP007stays off.Unionis still required as a runtime value for the public type aliases (PathLike,ChoicesType,AppRef,TableData,WidgetRef) and forUnion[args]construction. ruff offers no fix for those 11 sites — one of them,WidgetClass, holds string forward refs where|would be a runtimeTypeError— so enabling it would only leave permanent lint errors.The rewrite itself is small: 3 files, all genuine annotation positions.
docs/examples/demo_widgets/optional.pynow usesstr | None, which meanstest_examples.pyexercises the fix end to end.Also included: a lint fix for main
mainis currently failing ruff. #742 addedfrom typing import Callableto the ipynb backend and merged after #745 switchedtarget-versiontopy311, so its CI ran against the old config and never sawUP035.Related issues
I searched the tracker; none of these report this bug, and this PR does not close any of them. Listing them because they're adjacent and were candidates:
Union#93 — feature request for swappable Union widgets, unrelatedOptionalwidget-mapping and return-handling, bothtyping.OptionalguiclasswithOptional[int] = None. Still fails after this PR (TypeError: float() argument must be...); it's a value-coercion bug, not an origin bug. What this PR does guarantee is thatOptional[int]andint | Nonenow fail identically there — previously they diverged.Testing
Two regression tests in
tests/test_types.py, both confirmed failing before the fix and passing after. Full suite green on 3.11 / 3.13 / 3.14 across PyQt6, PyQt5, PySide6 (439 passed).🤖 Generated with Claude Code